Go to definition in classpath #23

olafurpg · 2017-11-10T00:32:02Z

This PR implements a Ctags module that is able to build semanticdbs for Scala and Java source files in the classpath, without any compilation or analyzing classfiles. The scala indexer uses the scalameta parser to find definitions of global symbols and record the positions at the rate of ~30k lines/s (see scalameta/scalameta#1130 for idea how to speed parsing by up to 40%). The Java indexer uses qdox which goes through java sources at ~250k loc/s on my laptop. qdox specifically designed for performance in mind and does not even parse bodies of methods or other metadata that scalameta parser collects.

We store the full file contents of all source files in the classpath in-memory. This goes against one the the core goals of this project, which is to keep memory usage as low as possible.

This commit makes it possible to quickly index a classpath of a project. This makes it possible to jump to definition from a project source file to a dependency source file. Note that this does not make is possible to jump to definition from dependency sources to dependency sources. While testing this feature, I extended the Jars utility to fetch -sources jars instead of regular jars and adapted the scala.meta.Classpath.deep method to read entries of jar files and report errors using our logging infrastructure.

The implementation in this commit is very hacky since I'm not sure how we can jump to definition inside entries of jars. I'm sure it's possible to just give the jar uri or something and everything automagically works. We index the entire classpath for every entry in the project/config matrix without any deduplication. This needs to be optimized. Most of the work was to get the compiler to correctly download the sources jars from the classpath. I had to wrap the call to `Jars.fetch` inside of a `Future` to unblock the compiler for future updates. This is a bug that needs to be fixed, we should not be randomly sprinkling blocks inside `Future { ... }`.

Previously we would re-index the same jars for every project/config and on every configuration change. Now we only index each jar once.

The positions are off and this implementation doesn't handle static methods.

It does not scale to have ctags indexing for all languages in a single file. The commit abstracts over the commonalities between scala and java indexing so that we can reuse most code between the two.

I've started getting MetaspaceErrors and I suspect they're related to us not properly cleaning up resources. Maybe this will fix the problem.

olafurpg · 2017-11-11T13:23:49Z

Go to definition for java sources in the classpath works!

It doesn't handle all cases like static methods or inner classes. JDK sources are not indexed either, I think we have to special case it. However, this is a very promising start, as long as we can move the editor to a java source file we can delegate the rest to the Java LSP.

I started getting MetaSpaceErrors, which is not good. The last commit adds one missing .close() which I hope solves the problem, if not we need to figure out what is causing those since it's very annoying to have to restart sbt every few minutes (and we don't want the server leaking memory either!)

olafurpg · 2017-11-11T13:26:02Z

The JavaCtags module is quite primitive for now. I struggled to get nice positions for names, it seems they were all offsets. The infrastructure is there however to index java sources. In fact, we can add any other language for that matter! It should be possible for example to support go to definition in protobuf/thrift/avro sources.

ShaneDelmore · 2017-11-11T16:30:52Z

metaserver/src/main/scala/scala/meta/languageserver/Ctags.scala

+  def index(filename: String, contents: String): Document = {
+    val input = Input.VirtualFile(filename, contents)
+    val tree = {
+      import scala.meta._


Lines 13-24 import various bits of scala.meta, then here you import all. Why? Is the style no wildcards allowed at top of file?

ShaneDelmore · 2017-11-11T16:35:27Z

metaserver/src/main/scala/scala/meta/languageserver/Compiler.scala

 import java.nio.file.Files
 import java.util.Properties
+import java.util.concurrent.ConcurrentHashMap
+import java.util.concurrent.ConcurrentHashMap
+import java.util.concurrent.ConcurrentHashMap


What caused this to be repeated 3 times? Some automated tool?

Fixed with OrganizeImports

ShaneDelmore · 2017-11-11T16:37:36Z

metaserver/src/main/scala/scala/meta/languageserver/Compiler.scala

    extends LazyLogging {
+  private val documentPubSub =


This name makes me think it is (Publisher, Subscriber) but based on the usage it is actually (Subscriber, Publisher)

Previous we could still overindex in race conditions, now we use Map.computeIfAbsent to properly make sure we only index each jar once. Race conditions are very possible since multiple .compilerconfig are often created at the same time.

ShaneDelmore · 2017-11-11T16:42:11Z

metaserver/src/main/scala/scala/meta/languageserver/CompilerConfig.scala

+package scala.meta.languageserver
+
+import java.io.File
+import java.nio.file.Files


Why are we using IO and NIO?

java.io is only for File.pathSeparator, otherwise we use nio for I/O work

NIO Filesystem has a platform independent separator if you want to stick with the NIO api.

I think I prefer java.io.File.pathSeparator over FileSystems.getDefault.getSeparator

olafurpg

Thanks for the review @ShaneDelmore !

olafurpg · 2017-11-11T16:45:31Z

metaserver/src/main/scala/scala/meta/languageserver/Compiler.scala

 import java.nio.file.Files
 import java.util.Properties
+import java.util.concurrent.ConcurrentHashMap
+import java.util.concurrent.ConcurrentHashMap
+import java.util.concurrent.ConcurrentHashMap


Fixed with OrganizeImports

olafurpg · 2017-11-11T16:45:37Z

metaserver/src/main/scala/scala/meta/languageserver/Compiler.scala

    extends LazyLogging {
+  private val documentPubSub =


olafurpg · 2017-11-11T16:46:34Z

metaserver/src/main/scala/scala/meta/languageserver/CompilerConfig.scala

+package scala.meta.languageserver
+
+import java.io.File
+import java.nio.file.Files


java.io is only for File.pathSeparator, otherwise we use nio for I/O work

ShaneDelmore · 2017-11-11T18:22:49Z

metaserver/src/main/scala/scala/meta/languageserver/Positions.scala

+      )
+  }
+
+}


An alternative suggestion in a completely different style. Nothing wrong with this one though if you want to keep it. Ignore my lack of filename in the IllegalArgumentExceptions, I just whipped this up in a worksheet.

def pos(content: String, line: Int, col: Int): Int = { val (lineCount, pos) = content .linesWithSeparators .take(line + 1) .foldLeft((0, 0)){ (accum, str) => val (lineNum, charCount) = accum if (lineNum < line) { (lineNum + 1, charCount + str.length) } else { require(str.length >= col, "Line is not long enough") (lineNum + 1, charCount + col) } } require(lineCount == line + 1, "Not enough lines") pos }

This method is on a fairly critical path since it's run for every start/end point during completions. foldLeft + tuples creates a lot of unnecessary allocations compared to the while loops.

However, I just found out that scala.meta.Input has an internal private lazy val cachedLineIndices: Array[Int] which allows you to get the offset of a line in from an array lookup. I've refactored to use this instead of rolling our own.

ShaneDelmore · 2017-11-11T18:30:11Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaEnrichments.scala

@@ -27,6 +27,7 @@ object ScalametaEnrichments {
    }
  }
  implicit class XtensionInputLSP(val input: m.Input) extends AnyVal {
+


Accidental new line?

- static classes - static methods

- handle enums in java - reuse test suite for java/scala

ShaneDelmore · 2017-11-12T16:37:37Z

metaserver/src/main/scala/scala/meta/languageserver/Effects.scala

+ *
+ * Observable[Unit] is not descriptive of what the observable represents.
+ * Instead, we create Unit-like types to better document what effects are
+ * flowing through our application.


Poor man's type-safety. I refactored to this in fact because I hit on bugs related to accidentally creating Observable[Observable[T]] from a .map when I should have been using .flatMap to build Observable[Unit]

are these phantom types?

Not really since those values also exist at runtime. I'm still not sure if this was necessary, maybe we can avoid it by using stricter scalac options (no discard unit). However, I think it's a nice replacement for Observable[Unit].

The JDK needs to be special handled since it's included in the classpath by default and does not appear in the `updateClassifiers` report from sbt. The JDK on my machine contains ~2.5M loc but still indexes in ~8s at a staggering rate of ~450k loc/s. The need to persist these indices across editor sessions grows bigger.

gabro

I went through the whole PR and I think I understand the most of it. Very nice work @olafurpg!

I've left some comments and questions, but overall I think we can merge this and improve things in separate issues.

I think the greatest improvements (maybe for a future PR) would be:

preload .compilerconfig files (easy)
make the jump to sources less hacky (without copying the file). I don't fully understand what Java LSP does, but that seems LSP-compliant

gabro · 2017-11-14T09:10:25Z

metaserver/src/main/scala/scala/meta/languageserver/Formatter.scala

  def classloadScalafmt(version: String, out: PrintStream): Formatter = {
    val urls = Jars
      .fetch("com.geirsson", "scalafmt-cli_2.12", version, out)
      .iterator
      .map(_.toURI.toURL)
      .toArray
+    out.println(s"Classloading scalafmt with ${urls.length} downloaded jars")


why are you printing to out instead of using the logger?

Because this is actually run before we redirect stdout, so if we use logger we it crashes the vscode client. However, I clearly should just redirect stdout before loading scalafmt 😅

Fixed, removed out parameter.

gabro · 2017-11-14T09:12:54Z

metaserver/src/main/scala/scala/meta/languageserver/Jars.scala

+  def fetch(
+      modules: Iterable[ModuleID],
+      out: PrintStream,
+      sources: Boolean


bikeshedding here, but sources isn't very descriptive. How about: fetchSources or shouldFetchSources, or similar?

Done, fetchSourceJars.

gabro · 2017-11-14T09:21:37Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaLanguageServer.scala

+    logger.info(s"File $path changed, extension=$name")
+    name match {
+      case "semanticdb" => semanticdbSubscriber.onNext(path)
+      case "compilerconfig" => compilerConfigSubscriber.onNext(path)


should we also preload all existing compilerconfigfiles at launch, similarly to what we do with semanticdb files?

We do actually, since we call this method from loadAllSemanticdbs... method, I've renamed the method to be more descriptive.

NB, we should really run that method asynchronously from the initialize method since it slows down server startup.

Yeah, in general I think initialize should return immediately and run the rest of the work asynchronously. We can even dynamically register for capabilities when the single services finish initializing.

gabro · 2017-11-14T09:25:12Z

metaserver/src/test/scala/scala/meta/languageserver/ctags/BaseCtagsTest.scala

+  def check(filename: String, original: String, expected: String): Unit = {
+    test(filename) {
+      val obtained = Ctags.index(filename, original)
+      println(obtained)


forgot a println

gabro · 2017-11-14T09:25:46Z

metaserver/src/test/scala/scala/meta/languageserver/ctags/ClasspathCtagsTest.scala

+class ClasspathCtagsTest extends FunSuite with DiffAssertions {
+
+  // NOTE(olafur) this test is a bit slow since it downloads jars from the internet.
+  test("index classpath") {


should we group this under a ci-slow target to run separately (like we do in Scalafix?)

I like the idea of a CI slow category as well.

gabro · 2017-11-14T09:34:38Z

metaserver/src/main/scala/scala/meta/languageserver/Compiler.scala

+    ctags.Ctags.index(sourcesClasspath) { doc =>
+      documentSubscriber.onNext(doc)
+    }
+    import scala.collection.JavaConverters._


what is this import for?

It's unused, removed.

gabro · 2017-11-14T09:36:04Z

metaserver/src/main/scala/scala/meta/languageserver/CompilerConfig.scala

+    sourceJars: List[AbsolutePath]
+) {
+  override def toString: String =
+    s"CompilerConfig(" +


since we have pprint as a dependency, you can probably just do pprint.stringify(this)

I can't find pprint.stringify, it seems to have been removed in 0.5. I don't think pprint includes the names of the fields. This string appears in the logs whenever a new presentation compiler is loaded so I customized it to make it compact and readable (full classpath can be huge).

gabro · 2017-11-14T09:48:02Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaLanguageServer.scala

-      MulticastStrategy.Publish,
-      OverflowStrategy.ClearBuffer(2)
-    )
+    Observable.multicast[AbsolutePath](MulticastStrategy.Publish)


why is the overflow strategy gone?

I added it without knowing what it does. I think we should use the default unless we know what we're doing.

gabro · 2017-11-14T09:56:58Z

metaserver/src/main/scala/scala/meta/languageserver/SymbolIndexer.scala

+  // However, that is a vscode only solution and we'd like this work for all
+  // text editors. Therefore, we write instead the file contents to disk in order to
+  // return a file: uri.
+  private def createFileInWorkspaceTarget(


Just as a comparison, the Java LSP jumps to a .class file containing the source.
I'm not sure I understand the implementation, but here's the relevant bit https://github.com/eclipse/eclipse.jdt.ls/blob/2127f12fb3700dd783b75d973e4f2ecc134b9ddf/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/NavigateToDefinitionHandler.java#L52

Nice find! I opened #36 to track this.

Added note in code.

gabro · 2017-11-14T09:59:55Z

metaserver/src/main/scala/scala/meta/languageserver/ctags/Ctags.scala

+  def index(input: Input.VirtualFile): Document = {
+    logger.trace(s"Indexing ${input.path} with length ${input.value.length}")
+    val indexer: CtagsIndexer =
+      if (isScala(input.path)) ScalaCtags.index(input)


what would it take to index .sbt files?

Not much really, I can give some pointers if you open an issue 😉

- Move overly deeply nested methods defined inside indexRoot(). Diff is huge, but it's only indentation. - Catch NPE which happens in the JDK, don't want it to appear in the logs every time you start the language server.

Previously the logs contains a ton of "File ...*.class changed", which is not useful.

The handling of default fields was apparently wrong, so I skipped the tests. I also skip the slow tests that do heavy IO. We can consider adding a IntegrationTest or separate testsSlow project, but for now I want snappy tests and a small build.

olafurpg

Thank you for the detailed review @gabro this PR became way too big, I kept adding more stuff to it while waiting for the lsp4j port. I will keep the PRs smaller from now on 😄

Some unattended changes

slow tests, we ignore those for now. We can add a separate project or config later. I don't want to use scalatest Slow tags for this since I want metaserver/test to be snappy by default.
jump to classpath, delegated to Figure out how to jump to files in zip/jar/.class files #36

olafurpg · 2017-11-14T18:40:28Z

build.sbt

      "io.get-coursier" %% "coursier" % coursier.util.Properties.version,
      "io.get-coursier" %% "coursier-cache" % coursier.util.Properties.version,
      "ch.epfl.scala" % "scalafix-cli" % "0.5.3" cross CrossVersion.full,
-      "com.geirsson" %% "scalafmt-core" % "1.3.0"
+      "org.scalatest" %% "scalatest" % "3.0.3" % "test",


Done, this was just copy pasted from above, I normally use Test

olafurpg · 2017-11-14T18:41:50Z

metaserver/src/main/scala/scala/meta/languageserver/Compiler.scala

+    ctags.Ctags.index(sourcesClasspath) { doc =>
+      documentSubscriber.onNext(doc)
+    }
+    import scala.collection.JavaConverters._


It's unused, removed.

olafurpg · 2017-11-14T18:43:57Z

metaserver/src/main/scala/scala/meta/languageserver/CompilerConfig.scala

+    sourceJars: List[AbsolutePath]
+) {
+  override def toString: String =
+    s"CompilerConfig(" +


I can't find pprint.stringify, it seems to have been removed in 0.5. I don't think pprint includes the names of the fields. This string appears in the logs whenever a new presentation compiler is loaded so I customized it to make it compact and readable (full classpath can be huge).

olafurpg · 2017-11-14T18:45:14Z

metaserver/src/main/scala/scala/meta/languageserver/Effects.scala

+ *
+ * Observable[Unit] is not descriptive of what the observable represents.
+ * Instead, we create Unit-like types to better document what effects are
+ * flowing through our application.


Not really since those values also exist at runtime. I'm still not sure if this was necessary, maybe we can avoid it by using stricter scalac options (no discard unit). However, I think it's a nice replacement for Observable[Unit].

olafurpg · 2017-11-14T18:46:20Z

metaserver/src/main/scala/scala/meta/languageserver/Formatter.scala

  def classloadScalafmt(version: String, out: PrintStream): Formatter = {
    val urls = Jars
      .fetch("com.geirsson", "scalafmt-cli_2.12", version, out)
      .iterator
      .map(_.toURI.toURL)
      .toArray
+    out.println(s"Classloading scalafmt with ${urls.length} downloaded jars")


Because this is actually run before we redirect stdout, so if we use logger we it crashes the vscode client. However, I clearly should just redirect stdout before loading scalafmt 😅

olafurpg · 2017-11-14T19:37:27Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaLanguageServer.scala

+    logger.info(s"File $path changed, extension=$name")
+    name match {
+      case "semanticdb" => semanticdbSubscriber.onNext(path)
+      case "compilerconfig" => compilerConfigSubscriber.onNext(path)


NB, we should really run that method asynchronously from the initialize method since it slows down server startup.

olafurpg · 2017-11-14T19:38:24Z

metaserver/src/main/scala/scala/meta/languageserver/SymbolIndexer.scala

+  // However, that is a vscode only solution and we'd like this work for all
+  // text editors. Therefore, we write instead the file contents to disk in order to
+  // return a file: uri.
+  private def createFileInWorkspaceTarget(


Nice find! I opened #36 to track this.

olafurpg · 2017-11-14T19:39:24Z

metaserver/src/main/scala/scala/meta/languageserver/SymbolIndexer.scala

+  // However, that is a vscode only solution and we'd like this work for all
+  // text editors. Therefore, we write instead the file contents to disk in order to
+  // return a file: uri.
+  private def createFileInWorkspaceTarget(


Added note in code.

olafurpg · 2017-11-14T19:40:19Z

metaserver/src/main/scala/scala/meta/languageserver/ctags/Ctags.scala

+  def index(input: Input.VirtualFile): Document = {
+    logger.trace(s"Indexing ${input.path} with length ${input.value.length}")
+    val indexer: CtagsIndexer =
+      if (isScala(input.path)) ScalaCtags.index(input)


Not much really, I can give some pointers if you open an issue 😉

olafurpg · 2017-11-14T19:40:52Z

metaserver/src/test/scala/scala/meta/languageserver/ctags/ClasspathCtagsTest.scala

+         |_root_.sourcecode.Util.getName. => def getName
+         |_root_.sourcecode.Util.isSynthetic. => def isSynthetic
+         |_root_.sourcecode.Util.isSyntheticName. => def isSyntheticName
+      """.stripMargin


YES I WANT DIS IN SCALA 🙏

olafurpg · 2017-11-14T20:11:29Z

MERGING 🚀

olafurpg · 2017-11-14T20:12:21Z

#35 is probably the biggest priority right now, since as you may notice, starting the server will always trigger reindexing the full JDK, even for trivial projects.

laughedelic · 2017-11-14T21:00:35Z

Does it happen on initialization or on the first definition request?

gabro · 2017-11-14T21:01:44Z

I think when compilerconfig is detected, so on initialization

olafurpg · 2017-11-14T21:28:50Z

It's triggered during initialization, but it does not block server initialize.

laughedelic · 2017-11-14T21:30:08Z

OK

laughedelic · 2017-11-26T05:06:59Z

metaserver/src/main/scala/scala/meta/languageserver/ctags/ScalaCtags.scala

+object ScalaCtags {
+  def index(input: Input.VirtualFile): CtagsIndexer = {
+    val root: Source = input.parse[Source].get
+    new Traverser with CtagsIndexer {


@olafurpg I was reading this code and I find it very similar to what I wanted to do for symbols outline. I started implementing something similar to this to traverse the tree just for the "global" symbols, but then I saw this and I'm wondering where is this Traverser from and how to use it. Direct search in Scalameta repo didn't give me a definition (I'm completely lost in Scalameta's project structure 🙁 )

@laughedelic I don’t have a computer in front of me to verify this but I believe some of the bits that you can’t tell where they are defined from scalameta are actually bits of the compiler plugin. Scala has a standard format for compiler plugins where they hook into a global content and supply their own traversed that the compiler uses to walk the ast and pull information out. It can appear a bit magic because it’s not fully scalameta code, but partially code for the plugin framework. I think traverser is one of these things.

For example, see this scala plugin demo project. Every one I have seen folllows a similar structure. https://github.com/cb372/scalac-plugin-basic/blob/master/src/main/scala/basic/BasicPlugin.scala#L56

In general, I think of scalameta as being intertwined with scalac so I don’t have to be, but when trying to understand scalameta source you will end up having to dig into scalac source as well as it is translating between the two ASTs.

@ShaneDelmore thanks a lot! This is quite enlightening. I didn't know anything about it. I'm not sure if I want to dig in the scalac's source just yet, but how can one use Scalameta without reading its sources? "_

@laughedelic I think Traverser is generated with macro annotations, it's essentially one big boilerplate pattern match on an ADT with 100s of nodes. Same for Transformer. The Traverser contract is quite simple however, override apply, do somthing with the tree node, optionally continue visiting children with super.apply(tree).

.collect is for example implemented on top of Traverser

scalameta doesn't convert between compiler data structures, the scalameta module doesn't depend on scala-compiler in fact. However, scalameta (ab)uses macro annotations so a lot of the source code is hidden inside quasiquotes to be expanded at compile time. To understand the macros it's helpful to know a bit about scalac

@laughedelic Here's how I picked up things about meta

https://github.com/scalameta/scalameta/blob/23de1ca839e53ad6387981bd7cced35122a2cf76/scalameta/trees/shared/src/main/scala/scala/meta/Trees.scala

http://scalameta.org/tutorial/#Trees

a lot of exploration in the ammonite repl

@ import $ivy.`org.scalameta::contrib:2.0.0`, scala.meta._, contrib._ import $ivy.$ , scala.meta._, contrib._ @ q"trait A { val x: Int }" res1: Defn.Trait = Defn.Trait( List(), Type.Name("A"), List(), Ctor.Primary(List(), _, List()), Template(List(), List(), Self(_, None), List(Decl.Val(List(), List(Pat.Var(Term.Name("x"))), Type.Name("Int")))) )

https://astexplorer.net/#/gist/104048fb30df84e64a3e46d774f26b0f/f4b78594d5b1716f5d2821ef76e8d48b3fafc8ba

writing new docs on more detailed parts like Transformer/Traverser is on my TODO!

@olafurpg hey! thanks a lot for explaining the things. I'm using the same "exploratory" approach too, it's quite enough for simple uses.
I just often wonder what is available in general (not to reinvent the wheel): normally if there's a generated scaladoc, it may be enough or (more often) I just check the sources and see the "global symbols outline" 😄 but with this "macrofull" code it's kind of hard (I'm also very superficially familiar with macros).
Another thing that bothers me is that I don't understand how code is organized in the scalameta repo. Probably there is some development dcoumentation for it that I didn't find?

@ShaneDelmore btw, thanks for mentioning quasiquotes.md, I didn't notice it before 👍 (I had Trees.scala open instead 😅 )

laughedelic · 2017-11-26T05:15:44Z

metaserver/src/main/scala/scala/meta/languageserver/ctags/ScalaCtags.scala

+          case t: Defn.Object => term(t.name, OBJECT); Continue
+          case t: Defn.Def => term(t.name, DEF); Stop
+          case Defn.Val(_, Pat.Var(name) :: Nil, _, _) => term(name, DEF); Stop
+          case Defn.Var(_, Pat.Var(name) :: Nil, _, _) => term(name, DEF); Stop


@olafurpg What about declarations? For example if we have somewhere defined

trait T { type X val x: X }

X will be a Decl.Type and x a Decl.Val, so if later in the code we encounter something like def foo(t: T): t.X and want to lookup that t.X... No, this example is bad, but I think you got the idea. If not, I'll think of a better example, sorry 😅

Those are actually not handled! This is a bug 😅

OK, I'll try to add it then.

Another question here: as I see from the definition of those term/type indexer methods they just use the name position, but for the outline we need the definition position instead. Do you think it could be also useful for the general indexer to use that wider position range? e.g.

case t: Defn.Def => term(t.name, t.pos, DEF)

Symbols outline would be then just a simple reindexing of the document on every documentSymbol request.

Do you think it could be also useful for the general indexer to use that wider position range? e.g.

Yes! I've been considering refactoring the indexer to construct a custom data structure that we can tweak for our needs. Then we can reuse it for multiple features like outline + go to definition.

Can you open a ticket to discuss details?

olafurpg added 10 commits November 10, 2017 17:28

Implement syntactic definition indexer for scala sources

ab517c3

Integrate ctags with compiler and symbol indexer.

eebbe39

Fix trailing comma and run tests in CI

062f8e9

Avoid redundant indexing.

cc537c5

Previously we would re-index the same jars for every project/config and on every configuration change. Now we only index each jar once.

Update lsp plugin

766361a

Get basic java ctags working.

864821d

The positions are off and this implementation doesn't handle static methods.

Refactor ctags into separate package.

4b175b9

It does not scale to have ctags indexing for all languages in a single file. The commit abstracts over the commonalities between scala and java indexing so that we can reuse most code between the two.

Close ZipInputStream.

6ad69f0

I've started getting MetaspaceErrors and I suspect they're related to us not properly cleaning up resources. Maybe this will fix the problem.

olafurpg force-pushed the classpath branch from 41fd1ea to 6ad69f0 Compare November 11, 2017 13:19

olafurpg requested a review from gabro November 11, 2017 13:26

Update readme

bf1b6f9

ShaneDelmore reviewed Nov 11, 2017

View reviewed changes

Improve deduplication of indexing source jars.

420f11d

Previous we could still overindex in race conditions, now we use Map.computeIfAbsent to properly make sure we only index each jar once. Race conditions are very possible since multiple .compilerconfig are often created at the same time.

ShaneDelmore reviewed Nov 11, 2017

View reviewed changes

olafurpg added 2 commits November 11, 2017 17:46

Address review feedback

e64a505

Clean up a few things

5c7951a

olafurpg commented Nov 11, 2017

View reviewed changes

olafurpg added 2 commits November 11, 2017 17:59

Fix compilation errors

bcd3905

Add more detailed explanation why we write jar: contents to disk

01f51b3

ShaneDelmore reviewed Nov 11, 2017

View reviewed changes

olafurpg added 3 commits November 11, 2017 23:54

Improve ctags indexing for java

db1779a

- static classes - static methods

Clean up ctags

3de1c85

- handle enums in java - reuse test suite for java/scala

Handle java fields

da8e1d8

ShaneDelmore reviewed Nov 12, 2017

View reviewed changes

laughedelic mentioned this pull request Nov 14, 2017

textDocument/definition implementation for VS Code. sbt/sbt#3660

Closed

2 tasks

gabro approved these changes Nov 14, 2017

View reviewed changes

olafurpg mentioned this pull request Nov 14, 2017

Persistable semanticdb search index #35

Closed

olafurpg added 2 commits November 14, 2017 20:15

Address review feedback

d05108d

Fix bug related to default java fields being static.

c2cb7bd

olafurpg mentioned this pull request Nov 14, 2017

Fixing symbols hierarchy #34

Merged

olafurpg added 7 commits November 14, 2017 20:29

Remove PrintStream from Formatter

020faaa

Clean up java ctags a bit.

81b8520

- Move overly deeply nested methods defined inside indexRoot(). Diff is huge, but it's only indentation. - Catch NPE which happens in the JDK, don't want it to appear in the logs every time you start the language server.

Use recommended parameter name

dc03def

Minimize log spam on server startup.

ed8e327

Previously the logs contains a ton of "File ...*.class changed", which is not useful.

Link issue to open files from jars.

fd2035a

Remove noise from ctags if shouldIndex filters a lot of paths

50191c1

Skip broken and slow tests.

3576919

The handling of default fields was apparently wrong, so I skipped the tests. I also skip the slow tests that do heavy IO. We can consider adding a IntegrationTest or separate testsSlow project, but for now I want snappy tests and a small build.

olafurpg commented Nov 14, 2017

View reviewed changes

Make JDK test pass regardless of JAVA_HOME filename length.

5e9272f

olafurpg merged commit 8bbef8e into scalameta:master Nov 14, 2017

olafurpg deleted the classpath branch November 14, 2017 20:11

laughedelic reviewed Nov 26, 2017

View reviewed changes

This was referenced Nov 29, 2017

Custom data structure for the index #72

Closed

Improve scala mtags #75

Merged

ckipp01 mentioned this pull request Jul 13, 2021

Metals locks up and becomes unresponsive with Scala 3 #2956

Closed

+                    )
+                }
+              }

Go to definition in classpath #23

Go to definition in classpath #23

Conversation

olafurpg commented Nov 10, 2017 • edited Loading

olafurpg commented Nov 11, 2017 • edited Loading

olafurpg commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Nov 14, 2017

olafurpg commented Nov 14, 2017

laughedelic commented Nov 14, 2017

gabro commented Nov 14, 2017

olafurpg commented Nov 14, 2017

laughedelic commented Nov 14, 2017

laughedelic Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

olafurpg commented Nov 10, 2017 •

edited

Loading

olafurpg commented Nov 11, 2017 •

edited

Loading

laughedelic Nov 26, 2017 •

edited

Loading

olafurpg Nov 26, 2017 •

edited

Loading

laughedelic Nov 27, 2017 •

edited

Loading

laughedelic Nov 27, 2017 •

edited

Loading